Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Kubernetes Manifests #5

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add Kubernetes Manifests #5

wants to merge 2 commits into from

Conversation

ddiiwoong
Copy link

Added manifest for docker-compose & Kubernetes environments

  • image repository : nexclipper/goul:0.2-head
  • tested on multi-node Kubernetes Cluster & Docker Desktop
  • use option "hostNetwork: true"

@ddiiwoong
Copy link
Author

잘지내시죠? 용환님! 저희 회사 프로젝트에서 필요해서 갑작스럽게 PR를 진행하게 되었습니다.

블로그에 사용 허락 요청을 드렸는데 안보시는거 같아서 PR로 연락드렸습니다.

검토해주시고 따로 연락한번 드리겠습니다.

@sio4
Copy link
Member

sio4 commented Aug 5, 2021

안녕하세요! 내용 확인해보고 연락 드리겠습니다!

sio4
sio4 reviewed Aug 7, 2021
View reviewed changes
cmd/goul/Dockerfile

WORKDIR /app
COPY . .
RUN go mod init nexclipper/goul
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just committed go.mod and go.sum (to make it compatible with recent go) so this line can make an error. Could you please remove this line?

sio4
sio4 reviewed Aug 7, 2021
View reviewed changes
cmd/goul/goul-client.yml
- name: GOUL_SERVER_ADDRESS
value: "172.16.9.126"
command: ["goul"]
args: ["--addr=$(GOUL_SERVER_ADDRESS)", "--debug", "--dev=$(GOUL_DEVICE)", "--port=$(GOUL_PORT)"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client container runs with no tcpdump filter so it will pass all packets (including STP and other management protocols) on the device to the server. If the server runs on the same LAN segment it could break the LAN or break the host network (by shutting down the switch port connected to the host by the switch that detects looping.)

@sio4
Copy link
Member

sio4 commented Aug 7, 2021 •
edited
Loading

I reviewed the PR and added some comments. In brief,

  1. I added go.mod and go.sum to make it compatible with a recent version of golang, so please remove the line accordingly. Otherwise, Dockerfile looks fine.
  2. For the other configurations, it could be harmful to the user if they do not carefully check those files and do not fully understand what it does. The user should understand how it can be dangerous when they just run it without modifying the configurations. As adding one more step for them, could you please move them all (except Dockerfile) into a new directory called /contrib and rename them with postfix .simple-example so the user can check the contents before using it? It could be somewhat grey-haired '90 style of source tree but I still love it. :-) If you add some WARNING comments on the file, it could be better too!

@sio4 sio4 self-assigned this Aug 7, 2021
@sio4 sio4 added the contrib label Aug 7, 2021
@sio4 sio4 force-pushed the master branch 2 times, most recently from 9dd6681 to 7a11125 Compare August 8, 2021 06:30
@sio4 sio4 removed the contrib label May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sio4 sio4 sio4 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@sio4 sio4

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ddiiwoong @sio4